add AliasAssign declarations#11846
Merged
atilaneves merged 1 commit intodlang:masterfrom Dec 14, 2020
Merged
Conversation
WalterBright
requested review from
RazvanN7,
UplinkCoder,
ibuclaw and
kinke
as code owners
Contributor
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#11846" |
WalterBright
force-pushed
the
AliasAssign
branch
from
8a3e3fd to
2e48598
Compare
thewilsonator
added
the
Review:Needs Spec PR
Member
Author
|
@thewilsonator I know, but most of that info is in the AliasAssign.md file, and I'll do the work for the spec PR once this is a go. |
Contributor
|
I'm more thinking documentation. |
WalterBright
force-pushed
the
AliasAssign
branch
2 times, most recently
from
a49fcf0 to
44ba28b
Compare
Member
Author
|
Buildkit heisenbug strikes again. Can't we fix these networking failures with "if it was a networking failure, sleep for a few seconds and try again?" |
WalterBright
force-pushed
the
AliasAssign
branch
from
44ba28b to
82e18b1
Compare
Member
|
Seems to stop just short of type functions? |
|
|
||
| --- | ||
| AliasAssign: | ||
| Identifier = Type; |
Contributor
There was a problem hiding this comment.
Why is reassignment limited to types? What about aliases for other symbols?
Member
Author
There was a problem hiding this comment.
Because if this doesn't work, it isn't worth the effort to extend it.
Member
|
You are rebuilding typefunctions.
We should talk about this.
Manipulating tupleexps directly is dangerous.
…On Sat, Oct 10, 2020, 1:45 PM Florian ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In changelog/AliasAssign.md
<#11846 (comment)>:
> + static foreach (t; T)
+ A = AliasSeq!(A, F!t); // alias assignment here
+ alias staticMap = A;
+}
+---
+
+Using the iterative approach will eliminate the combinatorial explosion of recursive
+template instantiations, eliminating the associated high memory and runtime costs,
+as well as eliminating the issues with limits on the nesting depth of templates.
+It will eliminate the obtuse error messages generated when deep in recursion.
+
+The grammar:
+
+---
+AliasAssign:
+ Identifier = Type;
Why is reassignment limited to types? What about aliases for other symbols?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#11846 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVWSCF3GJV6AR7QRXARC2LSKBCNRANCNFSM4SK6WCIQ>
.
|
Member
Really? There issue, that I have with this approach, is that I don't have proofs that guarantee this will not alter the langauge. Type-functions are essentially a shell around this ability, because this is shaky ground. |
Contributor
|
The changelog entry says:
How does this interact with on-demand semantic analysis of forward references? For example, what is the output of this program: template Example()
{
alias A = int;
alias B = S!string;
A value;
struct S(T)
{
A = T;
}
}
void main()
{
import std.stdio;
writeln(typeof(Example!().value).stringof);
}If we're going to add declarations that depend on the order of semantic processing, the language spec should specify what that order actually is. (Edit: added a template so that |
Member
Author
Yes, that's right. If it resolves the problem, then it's a much simpler solution. |
Member
Author
Compile time error, because the alias assignment's immediate parent is not the same template as the alias declaration. |
Member
Author
Member
Author
|
@UplinkCoder please provide a link to your description of type functions, and a link to your implementation code, and I will be happy to cite them as prior work. |
Contributor
|
I proposed something similar, but not exactly the same, in a couple of recent forum threads. [1][2] I'm not overly worried about attribution, but you're welcome to cite them if you like. [1] https://forum.dlang.org/thread/peatuezvcarhkjbsqqsv@forum.dlang.org?page=8#post-drybmowizcakksuvgraz:40forum.dlang.org |
Member
Author
|
@pbackus Your first cite looks like the same idea to me, though with the |
WalterBright
force-pushed
the
AliasAssign
branch
from
82e18b1 to
7667969
Compare
Member
Author
|
Azure pipelines (Windows_LDC_MinGW win32-ldc): I find this baffling. |
Member
It's your make ;) |
Member
The implementation is here: The spec is still upcoming. I can write a preliminary description though.
And that's it. |
Member
|
Nice. Not much time to look into this, but a few concerns. I'm trying to convince myself that this is removal of surprising limitations, as opposed to a large change. On the face of it, allowing reassignment of alias names is in keeping with the rest of D (mutability is the default). So I could imagine how we can consider that as removing a limitation. However, there are other things that D users expect. The problem is, as @pbackus noted, order dependency. Consider this code: alias X = int;
void f(X);
X = float;
void f(X);AFAIU this will compile into declarations
I think it would make sense to disallow reassigning an alias as soon as it's been "read", i.e. used in another declaration: That way (at least in such simple cases) the semantics of |
Member
Author
The difficulty with that is iteration such as from the example: Note the use of |
Member
Author
The trouble is that this AliasAssign only allows for assigning types to an alias. AliasDeclarations allow types and symbols to be aliased, hence the failures in the std.meta unittests. It's not a problem adding this code, except that this PR is already 440 lines and is getting fairly hard to review because of its size. Adding the symbol support is adding a lot of code. It would be much better to add the symbol support in a followup PR. |
WalterBright
force-pushed
the
AliasAssign
branch
from
6963cf6 to
3b07f1b
Compare
WalterBright
force-pushed
the
AliasAssign
branch
from
3b07f1b to
fa31879
Compare
Member
|
Where's the spec PR ? |
Member
|
Who approved this being merged? |
Contributor
|
@atilaneves have approve this. Also, I concur @Geod24 response here, this needs a spec pr. |
Member
Author
I'll add one, after I do the followup alias assign symbols. In the meantime, the changelog will do. BTW, could use some help on existing spec PRs. |
Geod24
reviewed
Dec 15, 2020
Member
Geod24
left a comment
Geod24
left a comment
There was a problem hiding this comment.
This SEGV the compiler:
import std.meta;
template Swap (alias A, alias B)
{
alias C = A;
B = A;
A = B;
enum Swap = true;
}
alias A = int;
alias B = string;
static assert(Swap!(A, B));
pragma(msg, A);|
|
||
| if (!aliassym.type) | ||
| { | ||
| ds.error("alias assignment can only apply to type aliases"); |
Member
There was a problem hiding this comment.
This prints:
test.d(9): Error: aliasAssign `test.staticMap!(Identity, int, string).__anonymous.__anonymous` alias assignment can only apply to type aliases
aliasAssign shouldn't be user-facing
| } | ||
| if (!aliassymParent.isTemplateInstance()) | ||
| { | ||
| ds.error("must be a member of a template"); |
Member
There was a problem hiding this comment.
Same here, this said "aliasAssign"
Member
There was a problem hiding this comment.
This also triggers when using mixin template.
|
|
||
| override const(char)* kind() const | ||
| { | ||
| return "aliasAssign"; |
Member
|
It would be great if we could stop putting experimental stuff straight into the language. |
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Dec 16, 2020
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Dec 17, 2020
Member
I'll ask again in a better way. Who approved this to be merged when there are still outstanding reviews to be addressed, some of which highlight broken code? |
Member
|
(Yes, this change caused a build regression in master). |
Contributor
|
@ibuclaw I missed the regression. How come CI didn't catch it? This was approved in a "let's see where this goes" fashion and might be reverted. |
Member
It's not something that can really be tested unless you pull in building ldc/gdc bootstrap into the fold (not feasible due to time it takes). Though the auto-generated frontend.h header highlighted many additions that simply weren't added, or were added incorrectly to the headers used by gdc and ldc. I pointed some of these out as well in my review. |
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Jan 3, 2021
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Jan 3, 2021
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Jan 3, 2021
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Jan 3, 2021
WalterBright
added a commit
to WalterBright/dmd
that referenced
this pull request
Jan 5, 2021
dlang-bot
pushed a commit
that referenced
this pull request
Jan 15, 2021
This was referenced May 6, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
15 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This evolved from a conversation with Andrei when he asked "why not just allow assignment to an alias?" Why not, indeed:
becomes:
Combining this with D's already powerful features
static foreach, tuple slicing and tuple concatenation we should be able to re-implement thestd.metatemplates using this, and eliminating the recursion and recursion's problems.Implementing it turned out to be straightforward, once I figured out that making the
A = AliasSeq!(A, F!t);an anonymous Dsymbol was the way to go. Most of the implementation is all of the boilerplate needed to add a new Dsymbol type, the actual implementation is minimal.